Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ME: iso19139 spatial extents #944

Merged
merged 4 commits into from
Aug 6, 2024
Merged

Conversation

LHBruneton-C2C
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C commented Jul 22, 2024

Description

This PR introduces support of the spatial extents in the ISO converters.
Both bbox and the first of multiple geometries are supported.
The description is supported only through the MD_Identifier tag.

Architectural changes

The api-metadata-converter library now depends on OpenLayers.

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

Copy link
Contributor

github-actions bot commented Jul 22, 2024

Affected libs: api-metadata-converter, feature-search, feature-router, feature-map, feature-dataviz, feature-record, api-repository, feature-catalog, feature-editor, feature-auth, ui-search, common-domain, common-fixtures, ui-elements, feature-notifications, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-layout, ui-map, ui-dataviz,
Affected apps: metadata-converter, metadata-editor, datahub, demo, webcomponents, search, map-viewer, datafeeder, data-platform,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/iso19139-spatial-extents branch from 97a71f8 to 33e79e7 Compare July 22, 2024 14:14
@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review July 22, 2024 14:22
Copy link
Contributor

github-actions bot commented Jul 22, 2024

📷 Screenshots are here!

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/iso19139-spatial-extents branch from 33e79e7 to f924559 Compare July 24, 2024 08:54
@coveralls
Copy link

coveralls commented Jul 24, 2024

Coverage Status

coverage: 82.97% (+2.0%) from 80.977%
when pulling 0521005 on ME/iso19139-spatial-extents
into a1bcfe2 on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!! most of this looks really good. My main question is about the "bbox" property, which I wish we could avoid adding; using only geometries would make things simpler overall I think.

Also the logic for adding the GML namespace should not be needed, the XML document generation already adds all necessary namespaces at the top in the end :)

${parentPadding}`
}

function getGmlNamespaceAsNeeded(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need that; the namespaces are added at the end when producing the XML document, see:

export function createDocument(rootEl: XmlElement): XmlDocument {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the way I parse/serialize the GML part with OpenLayers, I don't have the full access to the XmlDocument at that time, as OpenLayers only takes in native node objects, I have to use a string as a intermediate step at some point.
It's there that I'm missing the GML namespace.
If you know a way to have OL understand the Xml elements from the lib we use to parse the record, I'd be really interested.
I agree adding this namespace manually is ugly (and the serialization is ugly too, we change default namespace in the middle of the tree, but apparently it's allowed, so...).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried a few things, I think this can be handled inside the readGeometry function without any change to xmlToString, see my other comment

libs/api/metadata-converter/src/lib/xml-utils.ts Outdated Show resolved Hide resolved
@LHBruneton-C2C
Copy link
Collaborator Author

using only geometries would make things simpler overall I think.

I mostly agree, but as we are dealing with an array of spatial extents, I remove all the elements before writting the new ones, which in this case would mean changing the content of the original record, even when doing only partial edition. Is it OK for the original record to switch from a BBOX spatial extent to a Polygon spatial extent?

@jahow
Copy link
Collaborator

jahow commented Jul 29, 2024

Is it OK for the original record to switch from a BBOX spatial extent to a Polygon spatial extent?

Hmm, I didn't think of that. Maybe there could be a way to figure out that a Polygon is in fact a bbox? something like so:

// this was generated with the help of chatgpt
function isBoundingBox(geom: Geometry) {
  if (geometry.type !== 'Polygon') {
    return false;
  }

  const coordinates = geometry.coordinates;

  // is it a rectangle polygon with no holes?
  if (coordinates.length !== 1 || coordinates[0].length !== 5) {
    return false;
  }

  const [first, second, third, fourth, last] = coordinates[0];
  
  // is it a closed ring?
  if (first[0] !== last[0] || first[1] !== last[1]) {
    return false;
  }

  // is it an axis-aligned rectangle?
  return first[0] === second[0] && first[1] === fourth[1] &&
    second[1] === third[1] && third[0] === fourth[0] &&
    first[0] !== third[0] && first[1] !== third[1];
}

@Angi-Kinas Angi-Kinas mentioned this pull request Jul 31, 2024
7 tasks
@LHBruneton-C2C
Copy link
Collaborator Author

Hmm, I didn't think of that. Maybe there could be a way to figure out that a Polygon is in fact a bbox? something like so:

So, I'm reading the PR on how to add new spatial extents, and in fact we will only add Polygons that are bboxes. So we end up with a record with several geometries, we first clean those from the XML as this is an array, but then we don't know which one was the original bbox, as all our added geometries are bboxes... It feels like keeping the bbox field in the model is less troublesome :/

})
findChildElement('gmd:polygon', false),
firstChildElement,
map((el) => readGeometry(el) as Polygon | MultiPolygon)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jahow Can you explain why this cast to Polygon | MultiPolygon, when the GML can be of any kind, and OL is parsing it correctly?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry this can be removed, it is indeed useless!

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/iso19139-spatial-extents branch from 986c82d to 2d17380 Compare August 2, 2024 09:08
@LHBruneton-C2C LHBruneton-C2C requested a review from jahow August 2, 2024 09:12
@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/iso19139-spatial-extents branch from 2d17380 to 0521005 Compare August 2, 2024 11:11
@LHBruneton-C2C
Copy link
Collaborator Author

Merging with e2e tests failure:

image

@LHBruneton-C2C LHBruneton-C2C merged commit 8118add into main Aug 6, 2024
8 of 11 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME/iso19139-spatial-extents branch August 6, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants